Skip to content

Conversation

@MitchBradley
Copy link

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds support for HTTP chunked transfer encoding in incoming requests to the ESPAsyncWebServer library. This enables clients to send request bodies without knowing the total content length in advance, which is particularly useful for WebDAV operations and large file uploads.

Changes:

  • Adds chunked transfer encoding parsing state machine to handle Transfer-Encoding: chunked requests
  • Introduces support for the non-standard X-Expected-Entity-Length header used by macOS WebDAVFS
  • Adds HTTP 507 "Insufficient Storage" status code for space-related errors

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
src/literals.h Adds X-Expected-Entity-Length header constant and HTTP 507 status code string
src/ESPAsyncWebServer.h Declares new member variables for chunk parsing state (_chunkStartIndex, _chunkOffset, _chunkSize, _chunkedParseState) and the _parseChunkedBytes method
src/WebRequest.cpp Implements chunked encoding state machine, adds Transfer-Encoding header detection, initializes chunked state variables, and routes chunked requests through new parsing logic
examples/ChunkRequest/ChunkRequest.ino Provides example demonstrating chunked PUT requests with file storage, including space checking and error handling
platformio.ini Adds reference to the new ChunkRequest example

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

_chunkedParseState = CHUNK_EXTENSION;
} else if (data == '\n') {
_chunkOffset = 0;
_chunkedParseState = CHUNK_DATA;
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The chunk length parser doesn't handle invalid characters. If a character that is not a hex digit (0-9, A-F, a-f), semicolon, or newline is encountered in CHUNK_LENGTH state, it is silently ignored. According to RFC 9112, invalid characters should cause the request to be rejected. Consider adding error handling for invalid characters, for example by transitioning to a PARSE_ERROR state or aborting the connection.

Suggested change
_chunkedParseState = CHUNK_DATA;
_chunkedParseState = CHUNK_DATA;
} else if (data != '\r') {
// Invalid character in chunk length: reject the request
_parseState = PARSE_REQ_FAIL;
return true;

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley : this one is valid I think, but the suggestion should be instead _chunkedParseState = CHUNK_ERROR; maybe ?

return true;
}
_chunkSize = 0;
_chunkedParseState = CHUNK_LENGTH;
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CHUNK_END state parser doesn't handle invalid characters. If a character other than newline is encountered in CHUNK_END state (which expects CRLF), it is silently ignored. This could lead to malformed chunks being accepted. Consider adding error handling for unexpected characters in this state.

Suggested change
_chunkedParseState = CHUNK_LENGTH;
_chunkedParseState = CHUNK_LENGTH;
} else {
// Unexpected character in CHUNK_END state: abort parsing.
if (_client) {
_client->close();
}
return true;

Copilot uses AI. Check for mistakes.
Copy link
Author

@MitchBradley MitchBradley Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tough one. I added a CHUNK_ERROR state that will ignore the rest of the data stream, but I'm not sure how to signal the error to the application layer (handleBody). Any ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abort(); should be called (you can have a look at the many cases in the WebRequest class when parsing fails)

the app will not be notified because you are still in the parsing phase of the request and any failed parsing leads to an invalid request (client closed) thus request object deleted and its _tmp object freed (that is why the _tmp object should also always sadly be created with malloc in theory).

the only way to be notified about a request connection closed is to setup a callback request->onDisconnect() when the streaming starts (index == 0) and this callback can eventually correctly delete _tmp object if it was created by new and set it to null to avoid the class destructor handle the destruction with free. Other cleanups can be done also, but at this point there is no answer to create.

That's similar to other use cases we have in the parsing code where we call abort()

@MitchBradley
Copy link
Author

I think it's ready for the next step. The remaining comments from Copilot don't seem worthwhile.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +430 to +439
if (data == '\n') {
// A zero length chunk marks the end of the chunk stream
if (_chunkSize == 0) {
// If we needed to support trailers, we would switch to
// TRAILER state, but since we have no use case for them,
// we just stop processing the body.
return true;
}
_chunkSize = 0;
_chunkedParseState = CHUNK_LENGTH;
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CHUNK_END state doesn't properly handle the '\r' character before '\n'. According to RFC 9112, each chunk must be terminated by CRLF ("\r\n"), but this code will accept a bare LF ("\n"). While this is lenient and may work with most clients, it's not strictly HTTP-compliant. Consider verifying that '\r' precedes '\n' to ensure proper protocol adherence.

Suggested change
if (data == '\n') {
// A zero length chunk marks the end of the chunk stream
if (_chunkSize == 0) {
// If we needed to support trailers, we would switch to
// TRAILER state, but since we have no use case for them,
// we just stop processing the body.
return true;
}
_chunkSize = 0;
_chunkedParseState = CHUNK_LENGTH;
if (data == '\r') {
// Ignore CR; wait for LF to complete CRLF
} else if (data == '\n') {
// Ensure LF is preceded by CR within this buffer
if (i < 2 || buf[i - 2] != '\r') {
_chunkedParseState = CHUNK_ERROR;
} else {
// A zero length chunk marks the end of the chunk stream
if (_chunkSize == 0) {
// If we needed to support trailers, we would switch to
// TRAILER state, but since we have no use case for them,
// we just stop processing the body.
return true;
}
_chunkSize = 0;
_chunkedParseState = CHUNK_LENGTH;
}

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley : that's a valid improvement I think, to be more in line with the spec, although I would accept if we do not have it lol because all clients should correctly craft a request and if not then this is a user creating its own request like with AsyncTCP... Which IMO is not likely to happen. So I am fine with whatever you decide on this one.

Comment on lines +510 to +518
} else if (name.equalsIgnoreCase(T_Transfer_Encoding)) {
String lowcase(value);
lowcase.toLowerCase();

if (lowcase.indexOf("chunked") != -1) {
_chunkSize = 0;
_chunkStartIndex = 0;
_chunkedParseState = CHUNK_LENGTH;
}
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check for "chunked" in the Transfer-Encoding header uses indexOf, which performs a substring match. This could theoretically match incorrect values like "not-chunked" or "pre-chunked". According to RFC 9112, transfer codings should be compared as complete tokens. Consider using a more precise check that ensures "chunked" is a complete token, possibly by splitting on commas and comparing each token.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchBradley : oh boy... I agree and disagree... Maybe if found, check the character before if any to validate this is a space, comma or semi-colon ? I think only those could happen before right ?

mathieucarbou and others added 2 commits January 30, 2026 16:04
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants